fix-checked-arithmetic-footprint - #426

…oldii-locks#288)

…oldii-locks#288)

…oldii-locks#288)

…tests (Goldii-locks#290)

Harden cancel_escrow with two missing validation rules and add a full
test suite covering every guard, happy path, post-cancel state, event
structure, and milestone isolation.

Production changes (lib.rs):
- Add EmergencyPaused guard: cancel_escrow now returns Error::Paused when
  the contract is emergency-paused, consistent with all other user-facing
  endpoints
- Add duplicate-cancel guard: a second call while CancelLock is already
  active returns Error::EscrowLocked, preventing race conditions and
  redundant lock-sets

Tests added (test.rs) — 20 new tests:

  Invalid address guards:
    - test_cancel_escrow_zero_account_address_rejected
    - test_cancel_escrow_zero_contract_address_rejected

  Not-initialized guard:
    - test_cancel_escrow_not_initialized_fails

  Not-funded guard:
    - test_cancel_escrow_not_funded_fails

  Unauthorized guards:
    - test_cancel_escrow_stranger_unauthorized
    - test_cancel_escrow_arbiter_unauthorized
    - test_cancel_escrow_admin_unauthorized

  Emergency-paused guard:
    - test_cancel_escrow_while_paused_fails

  Duplicate-cancel guard:
    - test_cancel_escrow_duplicate_call_fails
    - test_cancel_escrow_freelancer_duplicate_after_client_fails

  Happy paths:
    - test_cancel_escrow_client_succeeds
    - test_cancel_escrow_freelancer_succeeds

  Post-cancel state validation:
    - test_cancel_escrow_blocks_fund
    - test_cancel_escrow_blocks_mark_delivered
    - test_cancel_escrow_blocks_approve_milestone
    - test_cancel_escrow_blocks_raise_dispute

  Event validation:
    - test_cancel_escrow_emits_exactly_one_event
    - test_cancel_escrow_event_contains_correct_caller

  Milestone state isolation:
    - test_cancel_escrow_does_not_mutate_milestones
    - test_cancel_escrow_all_milestones_released_still_succeeds

All 218 tests pass.

…nt-for-milestone

…tests (Goldii-locks#293)

Expand CancelEscrowInitiatedEvent with full operational context so
downstream indexers can reconstruct the complete cancellation state
from the event payload alone, without querying contract storage.

Production changes (lib.rs):
- Expand CancelEscrowInitiatedEvent with six new fields:
    caller_is_client bool   — true if initiator is the client, false if freelancer
    client           Address — registered client address
    freelancer       Address — registered freelancer address
    token            Address — escrow token contract address
    milestone_count  u32    — number of milestones at cancellation time
    total_amount     i128   — aggregate milestone total (pre-release)
- Update cancel_escrow event emission to populate all new fields,
  deriving caller_is_client from (caller == meta.client)

No other code changed.

Tests added (test.rs) — 14 new tests (396 -> 410):

  Event count:
    - test_cancel_escrow_event_emitted_exactly_once

  contract_id field:
    - test_cancel_escrow_event_contract_id_correct

  caller field:
    - test_cancel_escrow_event_caller_is_client_address
    - test_cancel_escrow_event_caller_is_freelancer_address

  caller_is_client role field:
    - test_cancel_escrow_event_caller_is_client_true_for_client
    - test_cancel_escrow_event_caller_is_client_false_for_freelancer

  client / freelancer / token fields:
    - test_cancel_escrow_event_client_field_correct
    - test_cancel_escrow_event_freelancer_field_correct
    - test_cancel_escrow_event_token_field_correct

  milestone_count field:
    - test_cancel_escrow_event_milestone_count_single
    - test_cancel_escrow_event_milestone_count_multiple

  total_amount field:
    - test_cancel_escrow_event_total_amount_correct_single_milestone
    - test_cancel_escrow_event_total_amount_correct_multi_milestone

  Full indexer round-trip:
    - test_cancel_escrow_event_full_indexer_parse

All 410 tests pass.

…ocks#298)

Audited tax_withholding_deductions's existing validation against this
issue's requirement ("assert that bad setups are rejected immediately
with descriptive error types") and found the existing coverage already
extensive: NotInitialized, NotFunded, InvalidMilestone, InvalidRatio,
InvalidAmount (zero balance, overflow), and InvalidStatus for
Released/Refunded milestones are all implemented and tested (test.rs,
tax_withholding_tests.rs).

One real gap: the status check only excluded Released/Refunded, not
Disputed — unlike raise_dispute_inner and resolve_dispute elsewhere in
this file, which both treat Disputed as its own case via an exhaustive
match. A disputed milestone's funds are meant to be frozen pending
resolve_dispute; tax_withholding_deductions could still compute and
persist a TaxWithholdingRecord for one, moving money around that freeze.
No existing test combined raise_dispute with tax_withholding_deductions,
so this was uncovered on both the implementation and test side.

Fixes the gap by converting the two equality checks to an exhaustive
match over MilestoneStatus (so a future new variant fails to compile
here instead of silently falling through as allowed), matching the
established pattern elsewhere in this file, and adds a test exercising
it via the existing dual-signature test fixture.

Not touched: admin_tax_withholding_deductions, a separately-implemented
sibling function (per its own doc comment, "arrived from a separate PR
under the same name") that has no status check at all — a larger gap,
but outside this issue's named scope (Module/Component:
tax_withholding_deductions).

Drop the redundant MilestoneReleased(index) temporary-flag write from
multisig_admin_override_release. The flag's only reader
(is_milestone_released_flag) is dead code, and the persisted Released
status on the milestone is the authoritative completion signal. This
reduces the distinct storage keys written by the call from three
(Milestone, MilestoneReleased, MultisigLocked) to two (Milestone,
MultisigLocked), matching multisig_admin_override_refund.

Adds test_multisig_admin_override_release_reduced_storage_footprint to
assert the temporary flag is no longer written while token transfer,
lock clearing, milestone state, and terminal re-entry rejection still
hold. Updates the affected snapshot fixtures accordingly.

Closes Goldii-locks#392

Guard the refund arithmetic so no input can cause a wrap or an unhandled
panic (issue Goldii-locks#395). Negative amount / released_amount and
released_amount > amount now return Error::InvalidAmount before any
arithmetic runs, complementing the existing checked_sub and remaining<=0
guards. The same guards are applied to the sibling
multisig_admin_override_release for consistency.

Adds a comprehensive suite to multisig_admin_override_refund_tests
asserting i128::MAX / i128::MIN operands return Error::InvalidAmount
(rather than panicking) and that valid amounts refund exactly
amount - released_amount, identical to prior behavior.

Closes Goldii-locks#395

…oldii-locks#407)

…pgrade (Goldii-locks#352)

…_approve (closes Goldii-locks#354)

Reorder multisig_approve so the signer-membership check runs before any
job/token ledger reads (job meta load, cross-contract balance call),
matching the guard ordering used elsewhere in the hardening series.
Add dedicated tests asserting unauthorized and empty-balance rejections
leave the proposal's approval bitmap unmutated.

…d tests (Goldii-locks#353)

Also fixes an unrelated pre-existing compile break in
admin_override_cancel_tests.rs (missing #![cfg(test)] gate and
inaccessible setup_funded_escrow helper) so the crate's test suite
can build and run at all.

…eLock overhead

- annotate test modules with #[cfg(test)] so the wasm build succeeds
  without dev-dependencies
- make setup_funded_escrow pub(crate) and fix test imports so sibling
  test modules can use it (Address::generate requires testutils trait)
- remove redundant admin.require_auth() before require_admin in
  admin_override_cancel_refund; require_admin already performs the
  signature check, and env-host 22.1.3 rejects the double auth with
  Error(Auth, ExistingValue)
- fund a terminal-state cancel test through the zero-balance boundary
  guard in cancel_escrow so its invalid-amount assertion stays intact
- add missing admin_override_cancel_tests snapshot files (untracked,
  would otherwise fail CI on a fresh checkout)

…lance

- add Error::EmptyBalance (=32) and assert_nonzero_balance helper
- reject emergency_pause_claim_refund while the contract token balance is
  zero, so an emergency settlement never attempts an empty transfer
- fund the initialized-escrow fixture mint via its own token id so the
  existing claim_refund split-math tests keep passing
- add a test asserting EmptyBalance is returned for a paused but unfunded
  escrow

- annotate test modules with #[cfg(test)] so the wasm build succeeds
  without dev-dependencies
- make setup_funded_escrow pub(crate) and fix test imports so sibling
  test modules can use it (Address::generate requires testutils trait)
- remove redundant admin.require_auth() before require_admin in
  admin_override_cancel_refund; require_admin already performs the
  signature check, and env-host 22.1.3 rejects the double auth with
  Error(Auth, ExistingValue)
- fund a terminal-state cancel test through the zero-balance boundary
  guard in cancel_escrow so its invalid-amount assertion stays intact
- add missing admin_override_cancel_tests snapshot files (untracked,
  would otherwise fail CI on a fresh checkout)

…e on-ledger footprint

Rename the emergency-pause instance storage keys to shorter symbols:
  EmergencyPaused      -> Ep   (2 chars vs 16)
  EmergencyPauseLock   -> EpLk (4 chars vs 19)

Applied consistently across the milestone-escrow contract and the
reports copy, and updated the affected test snapshots so the on-ledger
symbol assertions match the shorter keys.

This is an isolated, self-contained change (Closes Goldii-locks#323).

The storage change is sound: MilestoneTimeExtension moves from
persistent to temporary storage, which matches how the rest of this
contract already treats deadline-scoped state -- DeliveredAt is
temporary too, and extend_ttl is not used anywhere in the file, so no
new TTL obligation is introduced.

The branch also added `time_extension: 0` to a Milestone literal in
test.rs, but Milestone has only amount, released_amount, status and
delivered_at:

  error[E0560]: struct `Milestone` has no field named `time_extension`

Nothing else in the branch references such a field -- the extension is
keyed storage, not a struct member -- so the line was simply removed.

518 tests passing / WASM release build OK

…timize-storage-keys-footprint-for-milestone

fix: optimize milestone_time_extensions storage key footprint

# Conflicts:
#	contracts/milestone-escrow/src/lib.rs

…sed-milestone case

Two things kept this from compiling and passing.

The emergency-pause guard read DataKey::EmergencyPaused, which does not
exist:

  error[E0599]: no variant or associated item named `EmergencyPaused`
                found for enum `DataKey`

This contract has two separate pause flags -- DataKey::Ep for the
emergency pause (set by emergency_pause_admin_override, read by
ensure_not_paused) and DataKey::Paused for admin_pause_escrow. The
comment and the Error::Paused return both describe the emergency one,
so the guard now reads DataKey::Ep. The branch's own CancelLock check
is left as-is, since it returns EscrowLocked rather than the error
ensure_not_paused would give.

The new test test_cancel_escrow_all_milestones_released_still_succeeds
then failed. Its premise -- "no business rule blocks it" -- is no longer
true: cancel_escrow rejects a zero contract balance with InvalidAmount,
and releasing the only milestone empties the contract, so the balance
guard fired rather than anything to do with milestone status.

The test now mints 1 stroop back to the contract before cancelling, the
same workaround Goldii-locks#429 used for the same guard. That keeps it testing what
it claims -- that Released milestones do not themselves block a cancel --
instead of re-testing the balance guard.

538 tests passing / WASM release build OK

…ow-validation

feat(cancel_escrow): add business rule validations and comprehensive …

The storage non-mutation assertions are the point of this PR and are
kept in full: the rejected multisig_approval_init calls now also assert
the original threshold survives and the would-be signer was never
written.

The branch predates two tests that have since landed on main, and its
copy of test.rs reverts them:

  test_multisig_approve_unauthorized_fails
  test_multisig_approve_illegal_source_state_fails

Both cover approve-side guards -- an unregistered signer, and a
zero-balance source state -- and both assert the approval bitmap is left
untouched. Merging as-is took the suite from 538 to 536 while the PR
description said it was adding coverage, which is the kind of loss that
passes CI without complaint.

Restored verbatim from main. The remaining changes to
admin_override_cancel_tests.rs are rustfmt reflow plus a #![cfg(test)]
attribute, and were left as the branch had them.

538 tests passing / WASM release build OK

…tisig-approval-init-guards

fix: add storage non-mutation coverage to multisig_approval_init guar…

The conflict in test.rs was two branches each appending a module
declaration at the same spot; both are needed, so both are declared.

execute_transfer_swaps_admin_and_emits_event then failed on the event
tally (left: 0, right: 1). The event is emitted -- the admin key swap and
the pending-transfer removal above it both asserted correctly -- but the
count was read after client.get_pending_admin_transfer(), and
env.events().all() reflects the most recent contract invocation.

Moved the two event assertions directly after execute_admin_transfer;
the state assertions follow, unchanged. Same fix as on Goldii-locks#418, which hit
this in multisig_admin_override_refund_tests. No production code
touched.

543 tests passing / WASM release build OK

…347-execute-admin-transfer-guards

feat: harden caller authorization and precondition guards in execute_admin_transfer (closes Goldii-locks#347)

main guards tax_withholding_deductions with two equality checks that
reject Released and Refunded. This branch replaces them with an
exhaustive match that also rejects Disputed, which is the whole point of
Goldii-locks#298: a Disputed milestone's funds are frozen pending arbitration, so
computing and persisting a tax split for it here would move money around
a dispute that resolve_dispute is meant to gate.

Took the branch's side. It strictly widens main's guard -- everything
main rejected is still rejected -- and being exhaustive means a future
MilestoneStatus variant fails to compile here rather than silently
falling through as permitted.

544 tests passing / WASM release build OK

…8-tax-withholding-validation

fix(tax_withholding_deductions): reject Disputed milestones (Goldii-locks#298)

The conflict was just a module declaration; it now sits under
#[cfg(test)] alongside the others, matching what Goldii-locks#429 established.

Two compile fixes in the new suite:

- setup_funded_escrow was not in scope. Imported from crate::test, the
  same way admin_override_cancel_tests.rs does it.
- DataKey::YieldRateBps does not exist. admin_set_yield_rate persists
  the rate as the yield_rate field of the YieldConfig entry under
  DataKey::YieldConfig, so read_yield_rate reads that instead. None
  still means "never written", which is what the no-mutation cases want.

Three of the new tests then failed, and they were right to. They pause
with emergency_pause and expect admin_set_yield_rate to return Paused,
but it called only assert_not_paused, which reads DataKey::Paused --
the flag admin_pause_escrow sets. The emergency pause is a separate,
stronger freeze recorded under DataKey::Ep, and nothing was checking it
here, so a yield-rate change went straight through an emergency pause
while the weaker admin pause blocked it.

admin_set_yield_rate now rejects under either flag. That is the
hardening this PR set out to add; the tests had simply reached for the
pause that was not wired up. No existing test asserted the old
behaviour.

558 tests passing / WASM release build OK

…set-yield-rate-guards

feat: harden caller auth and precondition guards in admin_set_yield_rate

Dropping the EmergencyPauseLock dance from
emergency_pause_admin_override is correct and is the point of Goldii-locks#399: the
lock exists to close a reentrancy window around external calls, and this
path makes none -- it reads the flag, compares, writes it back, and
emits an event. EpLk is still taken by the paths that do call out.

Three names had to be corrected against the enum as it actually exists:

- DataKey::EmergencyPaused -> DataKey::Ep in lib.rs
- DataKey::EmergencyPauseLock -> DataKey::EpLk in the new test suite

One test then failed on the event tally. It calls the override twice and
expects the count to go 1 then 2, but env.events().all() reports the most
recent contract invocation rather than a running total, so the second
call reports 1. Adjusted to assert the second call emits exactly one
event, with the existing payload check confirming it is the new one.
This is the same env behaviour that Goldii-locks#418 and Goldii-locks#438 ran into.

571 tests passing / WASM release build OK

…-pause-admin-override-storage-footprint

Goldii-locks#399 - fix(emergency_pause_admin_override): remove unnecessary EmergencyPauseLock overhead

The branch's own copy of admin_pause_escrow does not parse. A line from
the previous version was left dangling after the new writes:

    .set(&DataKey::EmergencyPauseLock, &true);
  env.storage().instance().set(&DataKey::Paused, &true);
      .set(&DataKey::EpLk, &true);   <- orphaned continuation

That produced 90 errors, all downstream of "expected expression, found
`.`". DataKey::EmergencyPauseLock is also not a variant -- the enum
calls it EpLk -- in lib.rs and twice in the new test suite.

Rebuilt the function around the two guards this PR is actually for:
assert_emergency_pause_not_locked before any write, and an early return
when the escrow is already paused so a redundant call mutates nothing.
Because that early return now handles the repeat case, the inner
`if !already_paused` that main used to gate the event is redundant, and
the publish is unconditional inside the lock.

Two of the new tests then failed on event tallies. They read
pause_event_count after is_paused / is_lock_held, and those helpers go
through env.as_contract -- env.events().all() reports the most recent
invocation, not a running total. Reordered so the tally is read first.
The idempotency case now asserts the second call emits nothing at all,
which is what "no-op" means here. Same env behaviour as Goldii-locks#418, Goldii-locks#428
and Goldii-locks#438.

576 tests passing / WASM release build OK

…349-admin-pause-escrow-guards

feat: harden caller authorization and precondition guards in admin_pause_escrow (closes Goldii-locks#349)

…om pf_alloc_admin_override

Resolve the merge against main, which had already landed a narrower
version of this event under the `pf_ovr` topic.

The branch declared a second `PlatformFeeAllocationOverrideEvent` struct
(same name, plus `contract_id` and `locked`) and published it a second
time under a `pfovrride` topic after the allocation lock was released --
two identically named types will not compile, and the two publishes are
the same event emitted twice.

Unified to one struct carrying the branch's richer payload and one
publish, kept inside the lock guard where main emits it. Retained main's
already-published `pf_ovr` topic rather than renaming a topic that is
live on the default branch. Updated main's existing assertion for the
two new fields and repointed the branch's four new tests at `pf_ovr`.

580 tests pass; wasm32 release build is clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

feat(Goldii-locks#396): emit PlatformFeeAllocationOverrideEvent from pf_alloc_admin_override

…pf_alloc_admin_override

Resolve the merge against main. Three fixes were needed on top of the
branch:

* The branch predates Goldii-locks#434, so taking its rewritten
  pf_alloc_admin_override body verbatim would have silently dropped the
  PlatformFeeAllocationOverrideEvent that call now emits. Kept the
  single-write optimisation and the event.

* The branch deletes `mod admin_override_cancel_tests;` -- stale-branch
  damage that would drop a whole test module from the build. Kept ours.

* Its new test referenced `DataKey::EmergencyPauseLock`, which does not
  exist; the variant is the short key `EpLk`.

Also folded the branch's `setup_pf_alloc_escrow` helper together with
Goldii-locks#434's `setup_locked_pf_alloc` instead of landing two near-identical
copies.

591 tests pass; wasm32 release build is clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

 perf(Goldii-locks#397): reduce ledger storage footprint of pf_alloc_admin_override

…time extensions

Main had already migrated MilestoneTimeExtension writes to temporary
storage, so the branch's remaining substance is the read fallback --
`load_time_extension` now checks temporary first and falls back to the
persistent key. Without it, any escrow whose extension was written before
the migration reads back 0 and auto-releases on the unextended deadline.

Fixes on top of the branch:

* Both new i128-extreme tests wrote storage from outside a contract
  context, which panics in soroban-sdk 22 ("not accessible outside of a
  contract"). Wrapped the seeding writes in `env.as_contract`.

* The branch reorders `mod admin_override_cancel_tests;` above `mod test;`
  and drops the `#[cfg(test)]` attributes main added; kept ours.

593 tests pass; wasm32 release build is clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>